-
-
Notifications
You must be signed in to change notification settings - Fork 300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve API client usability #1368
Conversation
Does anybody know why the SQLite rollback migration fails and how I can fix it? |
Codecov Report
Additional details and impacted files |
I think CI failure is not related. (Only pgsql fails, MariaDB and locally SQLite work) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some (hopefully useful) comments, but somebody with a greater understanding of the api_key
feature should have a look (I've never used it or even bothered to fully understand it).
database/migrations/2022_06_12_075709_add_token_to_user_table.php
Outdated
Show resolved
Hide resolved
database/migrations/2022_06_12_075709_add_token_to_user_table.php
Outdated
Show resolved
Hide resolved
… set as a random value
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested - LGTM.
@qwerty287 can you check if you are happy with my changes? :) |
Please give me the chance to do a review, too, before merge. |
@ildyria in general yes, but adding the old api key for the admin means that an api key that had no permissions before now has admin permissions. I can fix it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No text (just a "review" such that GH remembers what I have already seen)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing which remains to be done now, is step-debugging through user authentication middleware.
While I believe that the code is correct, I am currently unable to write proper tests for it. Again, the problem is that the application object survives between tests. As we already have had this problem once or twice and posted this question laravel/framework#44086. In the past we could always help ourselves with some workaround, but in this case I am lost. I hope some answers. |
And another one: laravel/framework#44088 |
I fixed the tests. While Only two things remain: @qwerty287 please check if you are fine with my changes and if the PR still does what you want it to do. The issue #1368 (comment) needs to be addressed. |
How should I fix the sqlite issue? Don't remove the column if it's SQLite? Changes are fine otherwise. |
For the same issue in other cases, we decided to not delete the column for any DBMS in order to keep the tables consistent between SQLite, MySQL and PostgreSQL. Otherwise it will be a nightmare to clean it up later. Simply add the column to #1321 such that we do not forget about it. When I will be ready with an PR for #1462, the tables need an extensive change such that we can clean up all the postponed changes, too. |
Let's wait for the tests and merge then. |
Hmm GH Actions seems not to work 🤔 |
Authorization
header.User::getCurrent
which allows to get the user which is currently logged inCloses #1356